Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

refactor(rome_js_anaylze): introduce StaticValue enum for checking value of the JSX attributes or JS expressions #4342

Merged
merged 7 commits into from
Apr 7, 2023

Conversation

nissy-dev
Copy link
Contributor

@nissy-dev nissy-dev commented Apr 2, 2023

Summary

This PR introduces common logic for checking expression values based on #4073 and discussion. This is useful when implementing some aria rules that checks value of the JSX attributes.

After merge in this PR, I will refactor much more in the current codebases using this methods.

Test Plan

I need to keep the current tests passed. I updated useNumericLiterals/invalid.js.snap because the previous code didn't handle empty template string correctly.

Changelog

  • The PR requires a changelog line

Documentation

  • The PR requires documentation
  • I will create a new PR to update the documentation

@netlify
Copy link

netlify bot commented Apr 2, 2023

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 797c15a
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/643030bc059b5700082e1c7f
😎 Deploy Preview https://deploy-preview-4342--docs-rometools.netlify.app/playground
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@github-actions github-actions bot added A-Linter Area: linter A-Parser Area: parser labels Apr 2, 2023
@nissy-dev nissy-dev changed the title refactor(rome_js_parser): refactor(rome_js_parser): introduce StaticValue enum for checking static values in expression Apr 2, 2023
@github-actions
Copy link

github-actions bot commented Apr 2, 2023

Parser conformance results on ubuntu-latest

js/262

Test result main count This PR count Difference
Total 48647 48647 0
Passed 47582 47582 0
Failed 1065 1065 0
Panics 0 0 0
Coverage 97.81% 97.81% 0.00%

jsx/babel

Test result main count This PR count Difference
Total 40 40 0
Passed 37 37 0
Failed 3 3 0
Panics 0 0 0
Coverage 92.50% 92.50% 0.00%

symbols/microsoft

Test result main count This PR count Difference
Total 6093 6093 0
Passed 1783 1783 0
Failed 4310 4310 0
Panics 0 0 0
Coverage 29.26% 29.26% 0.00%

ts/babel

Test result main count This PR count Difference
Total 639 639 0
Passed 568 568 0
Failed 71 71 0
Panics 0 0 0
Coverage 88.89% 88.89% 0.00%

ts/microsoft

Test result main count This PR count Difference
Total 16740 16740 0
Passed 12797 12797 0
Failed 3943 3943 0
Panics 0 0 0
Coverage 76.45% 76.45% 0.00%

@nissy-dev nissy-dev changed the title refactor(rome_js_parser): introduce StaticValue enum for checking static values in expression refactor(rome_js_anaylze): introduce StaticValue enum for checking static values in expression Apr 2, 2023
@nissy-dev nissy-dev changed the title refactor(rome_js_anaylze): introduce StaticValue enum for checking static values in expression refactor(rome_js_anaylze): introduce StaticValue enum for checking value of the JSX attributes or JS expressions Apr 2, 2023
@nissy-dev nissy-dev marked this pull request as ready for review April 2, 2023 12:49
@nissy-dev
Copy link
Contributor Author

@MichaReiser This PR is based on your comments in #4073, so I would appreciate if you could review.

CHANGELOG.md Outdated

#### Other changes

- Refactor common logic for checking value of the JSX attributes or JS expressions [#4073](https://github.com/rome/tools/pull/4073)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't want to document internal changes that don't affect the users. From this PR, the only change for users is the fact that we fixed a case inside the useNumericLiterals rule

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the differences in CHANGELOG.md.

self.text()
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should document this API and add a doctest, like we do for other public APIs exposed by this crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added doctests for StaticValue and QuotedString.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter A-Parser Area: parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants